Skip to content
This repository has been archived by the owner on Sep 7, 2020. It is now read-only.

Feature/add device information tlv to topology response #705

Merged
merged 5 commits into from
Feb 11, 2020
Merged

Feature/add device information tlv to topology response #705

merged 5 commits into from
Feb 11, 2020

Conversation

mariomaz
Copy link
Collaborator

[TASK] Add Device Information TLV to Topology Response message #700

@ghost ghost assigned mariomaz Jan 21, 2020
ghost
ghost reviewed Jan 21, 2020
View reviewed changes
common/beerocks/bcl/source/network/network_utils.cpp Outdated Show resolved Hide resolved
common/beerocks/bcl/source/network/network_utils.cpp Show resolved Hide resolved
framework/tlvf/AutoGenerated/include/tlvf/ieee_1905_1/tlvDeviceInformation.h Show resolved Hide resolved
framework/tlvf/AutoGenerated/include/tlvf/ieee_1905_1/tlvDeviceInformation.h Show resolved Hide resolved
framework/tlvf/AutoGenerated/src/tlvf/ieee_1905_1/tlvDeviceInformation.cpp Show resolved Hide resolved
framework/tlvf/AutoGenerated/include/tlvf/ieee_1905_1/tlvDeviceInformation.h Show resolved Hide resolved
framework/tlvf/AutoGenerated/src/tlvf/ieee_1905_1/tlvDeviceInformation.cpp Show resolved Hide resolved
framework/tlvf/AutoGenerated/src/tlvf/ieee_1905_1/tlvDeviceInformation.cpp Show resolved Hide resolved
framework/tlvf/AutoGenerated/src/tlvf/ieee_1905_1/tlvDeviceInformation.cpp Show resolved Hide resolved
framework/tlvf/AutoGenerated/src/tlvf/ieee_1905_1/tlvDeviceInformation.cpp Show resolved Hide resolved
tomereli
tomereli approved these changes Jan 21, 2020
View reviewed changes
Copy link
Collaborator

@tomereli tomereli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit 3cf23b9 headline should be "bcl: utils: add method to get interface speed".
Also, the description should also explain a bit which TLV needs this information and for which interfaces we require to know the speed.

Other than that - LGTM, so approving. Any reason this still a draft?

framework/tlvf/yaml/tlvf/ieee_1905_1/tlvDeviceInformation.yaml Outdated Show resolved Hide resolved
framework/tlvf/AutoGenerated/include/tlvf/ieee_1905_1/tlvDeviceInformation.h Show resolved Hide resolved
agent/src/beerocks/slave/backhaul_manager/backhaul_manager_thread.cpp
std::shared_ptr<ieee1905_1::cLocalInterfaceInfo> localInterfaceInfo =
tlvDeviceInformation->create_local_interface_list();

ieee1905_1::eMediaType media_type = ieee1905_1::eMediaType::UNKNONWN_MEDIA;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all this can go to a static function:

static bool fill_iface_info(ieee1905_1::tlvDeviceInformation &info, const std::string iface, const sMacAddr &iface_mac);

Then call it - fill_iface_info(*info, bridge_info.iface, network_utils::mac_from_string(bridge_info.mac));

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still have to add a LocalInterfaceInfo field for each wireless interface. I'll refactor this code when I had the full picture. Promise.

@ghost ghost added this to the M1 - Certifiable agent with wired backhaul milestone Feb 3, 2020
@ghost
Copy link

ghost commented Feb 6, 2020

@mariomaz What's the status of this PR? Can it be merged soon?

@mariomaz mariomaz force-pushed the feature/add_device_information_tlv_to_topology_response branch from 8acd78b to eb992df Compare February 7, 2020 10:14
@mariomaz mariomaz marked this pull request as ready for review February 7, 2020 10:27
@mariomaz
Copy link
Collaborator Author

mariomaz commented Feb 7, 2020

I have addressed requested changes, rebased onto master and changed the state to "Ready for review".
"[TASK] Add Device Information TLV to Topology Response message #700" cannot be fully completed until standard commands can be sent with DWPAL (I've updated task description accordingly and added a TODO comment in code).
However, this PR could be already merged as is (if no further changes are required).

arnout
arnout approved these changes Feb 7, 2020
View reviewed changes
Copy link
Collaborator

@arnout arnout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One bug, one important comment about the disappearing todos, but everything can be fixed up easily, so approving already.

framework/tlvf/yaml/tlvf/ieee_1905_1/tlvDeviceInformation.yaml Show resolved Hide resolved
framework/tlvf/yaml/tlvf/ieee_1905_1/tlvDeviceInformation.yaml Outdated Show resolved Hide resolved
agent/src/beerocks/slave/backhaul_manager/backhaul_manager_thread.cpp Show resolved Hide resolved
agent/src/beerocks/slave/backhaul_manager/backhaul_manager_thread.cpp Show resolved Hide resolved
agent/src/beerocks/slave/backhaul_manager/backhaul_manager_thread.cpp Show resolved Hide resolved
common/beerocks/bcl/include/bcl/network/network_utils.h Outdated Show resolved Hide resolved
common/beerocks/bcl/source/network/network_utils.cpp Outdated Show resolved Hide resolved
@arnout arnout added the don't merge This PR is not ready for merge or review label Feb 7, 2020
@mariomaz mariomaz force-pushed the feature/add_device_information_tlv_to_topology_response branch from eb992df to dcdb761 Compare February 10, 2020 16:53
@ghost ghost removed the don't merge This PR is not ready for merge or review label Feb 11, 2020
@mariomaz
tlvf: fix DeviceInformation TLV
3457028 
The nested type in `tlvDeviceInformation` was written as struct. This
was done when TLVF didn't support nested classes yet. However, it must
be a class since it has dynamically-sized content. Convert struct to
class and change its name accordingly.

Rename fields that do not match with those defined in the IEEE 1905
standard. Rename fields which type and purpose is not clear enough.

Change type of media_info field from eMediaType to array of uint8_t
since it is a variable length array which length media_info_length
and contents depend on the value of media_type field.

Signed-off-by: Mario Maz <[email protected]>
@mariomaz
tlvf: autogenerate TLVs
8466247 
Signed-off-by: Mario Maz <[email protected]>
@mariomaz
agent: rename topology query message handler method to match message …
7a47b2c 
…name

Signed-off-by: Mario Maz <[email protected]>
@mariomaz
bcl: utils: add method to get ethernet interface speed
d5c4552 
Ethernet interface speed is required to compute the MediaType field
in different TLVs. It is also required to compute the PHY rate when
collecting link metrics.

Signed-off-by: Mario Maz <[email protected]>
@mariomaz
agent: add local interface field for wired interface
6d6ec35 
Signed-off-by: Mario Maz <[email protected]>
@mariomaz mariomaz force-pushed the feature/add_device_information_tlv_to_topology_response branch from 2d4db46 to 6d6ec35 Compare February 11, 2020 17:36
@mariomaz mariomaz merged commit c8faead into master Feb 11, 2020
@mariomaz mariomaz deleted the feature/add_device_information_tlv_to_topology_response branch February 11, 2020 17:50
@mariomaz mariomaz mentioned this pull request Mar 27, 2020
Closed
2 tasks
@mariomaz mariomaz linked an issue Apr 8, 2020 that may be closed by this pull request
[TASK] Add Device Information TLV to Topology Response message #700
Closed
2 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rmelotte rmelotte rmelotte left review comments

@tomereli tomereli tomereli approved these changes

@arnout arnout arnout approved these changes

@morantr morantr Awaiting requested review from morantr morantr is a code owner

Assignees

@mariomaz mariomaz

Labels
None yet
Projects
None yet
Milestone
M1 - Certifiable agent with wired bac...
Development

Successfully merging this pull request may close these issues.

[TASK] Add Device Information TLV to Topology Response message
4 participants
@mariomaz @arnout @tomereli @rmelotte